Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 0007-dispute-freshly-finalized.zndsl failing #3893

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Mar 29, 2024

Test started failing after 66051ad which enabled approval coalescing, that was expected to happen because the test required an polkadot_parachain_approval_checking_finality_lag of 0, which can't happen with max_approval_coalesce_count greater than 1 because we always delay the approval for no_show_duration_ticks/2 in case we can coalesce it with other approvals.

So relax a bit the restrictions, since we don't actually care that the lags are 0, but the fact the finalities are progressing and are not stuck.

@alexggh alexggh added the R0-silent Changes should not be mentioned in any release notes label Mar 29, 2024
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ordian do you have any concerns with this? If so we could turn off coalescing for this test. IIRC we had a good reason to test for a value of 0 here, or maybe I am wrong.

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. I'd probably add some sleep before this check to ensure finality is not lagging after some time.

@ordian ordian requested a review from Overkillus March 30, 2024 02:21

# Check lag - dispute conclusion
honest: reports polkadot_parachain_disputes_finality_lag is 0
honest: reports polkadot_parachain_disputes_finality_lag is lower than 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this metric also lagging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was let me figure it why.

Copy link
Contributor Author

@alexggh alexggh Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's expected with the way we compute polkadot_parachain_disputes_finality_lag, because DetermineUndisputedChain

block_descriptions: subchain_block_descriptions,
is called with the list of blocks that are already approved by approval-voting, so when we compute the disputes_finality_lag we already include in it the approval-voting-lag because the subchain number can't be higher than the highest approved block in approval-voting:
let lag_disputes = initial_leaf_number.saturating_sub(subchain_number);

@alexggh alexggh added this pull request to the merge queue Apr 1, 2024
Merged via the queue into master with commit e6bd920 Apr 1, 2024
139 of 143 checks passed
@alexggh alexggh deleted the alexaggh/the_first_time_it_failed branch April 1, 2024 10:45
pgherveou pushed a commit that referenced this pull request Apr 2, 2024
Test started failing after
66051ad
which enabled approval coalescing, that was expected to happen because
the test required an polkadot_parachain_approval_checking_finality_lag
of 0, which can't happen with max_approval_coalesce_count greater than 1
because we always delay the approval for no_show_duration_ticks/2 in
case we can coalesce it with other approvals.


So relax a bit the restrictions, since we don't actually care that the
lags are 0, but the fact the finalities are progressing and are not
stuck.

Signed-off-by: Alexandru Gheorghe <[email protected]>
Ank4n pushed a commit that referenced this pull request Apr 9, 2024
Test started failing after
66051ad
which enabled approval coalescing, that was expected to happen because
the test required an polkadot_parachain_approval_checking_finality_lag
of 0, which can't happen with max_approval_coalesce_count greater than 1
because we always delay the approval for no_show_duration_ticks/2 in
case we can coalesce it with other approvals.


So relax a bit the restrictions, since we don't actually care that the
lags are 0, but the fact the finalities are progressing and are not
stuck.

Signed-off-by: Alexandru Gheorghe <[email protected]>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Apr 9, 2024
Test started failing after
paritytech@66051ad
which enabled approval coalescing, that was expected to happen because
the test required an polkadot_parachain_approval_checking_finality_lag
of 0, which can't happen with max_approval_coalesce_count greater than 1
because we always delay the approval for no_show_duration_ticks/2 in
case we can coalesce it with other approvals.


So relax a bit the restrictions, since we don't actually care that the
lags are 0, but the fact the finalities are progressing and are not
stuck.

Signed-off-by: Alexandru Gheorghe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants